Skip to content

ASD STIG / NIST 800-53 review of untrusted-input boundaries with remediations - #17

Closed
erikrozi wants to merge 8 commits into
masterfrom
devin/1788814351-asd-stig-nist-review-remediation
Closed

erikrozi wants to merge 8 commits into
masterfrom
devin/1788814351-asd-stig-nist-review-remediation

Conversation

@erikrozi

@erikrozi erikrozi commented Sep 7, 2026

Copy link
Copy Markdown

Closes N/A (security review; no linked issue)

Describe your changes:

Source-code review of Open MCT's untrusted-input and data boundaries against the DISA ASD STIG V6R4 (2025-09-09) and NIST SP 800-53 Rev. 5, plus remediation of the highest-value findings with Karma specs. The full assessment, including every boundary's explicit outcome and the deployment evidence an assessor would need, is in docs/security/asd-stig-nist-800-53-review.md. This is a code review, not a certification or attestation of compliance.

Findings already addressed by #10 (Comps mathjs), #11 (Web Page iframe sandbox / URL allowlist), #12 (npm registry signatures) and #13 (CSV formula injection) are excluded and not duplicated.

Findings table

Severity is the ASD STIG category of the cited rule. Line numbers refer to this branch.

ID File:line Description CWE ASD STIG V6R4 NIST 800-53 r5 Sev Outcome
F-01 importFromJSONAction/ImportFromJSONAction.js:87 Import tree parsed and persisted with only a __proto__ filter; constructor/prototype keys, identifier/key mismatches, non-string types, bad composition/condition-set references and unknown roots accepted CWE-1321, CWE-20 V-222606 SI-10 II not-satisfied -> remediated
F-02 ImportFromJSONAction.js:106,452 Import failures surfaced parser/provider text to operator CWE-209 V-222610 SI-11 II not-satisfied -> remediated
F-03 ImportFromJSONAction.js:348-350 Objects saved while walking the tree; later validation failure left partial state CWE-20 V-222609 SI-10(3) I not-satisfied -> remediated
F-04 exportAsJSONAction/ExportAsJSONAction.js:102-107 Export failure rethrew raw errors to UI CWE-209 V-222610 SI-11 II not-satisfied -> remediated
F-05 notebook/components/NotebookEntry.vue:460-479 Sanitizer schema allowed data:/protocol-relative links; link allowlist matched hostname suffixes; hand-built anchor did not escape text/href CWE-79 V-222602 SI-10 I not-satisfied -> remediated
F-06 NotebookEntry.vue:571, NotebookComponent.vue:676, notebook-image.js:72 Raw error interpolated into operator notification CWE-209 V-222610 SI-11 II not-satisfied -> remediated
F-07 persistence/couch/CouchObjectProvider.js:233-285 Network, HTTP and malformed-response failures propagated raw fetch/CouchDB text CWE-209, CWE-755 V-222610, V-222656 SI-11, SA-15(5) II not-satisfied -> remediated
F-08 localStorage/LocalStorageObjectProvider.js:37-105 Browser storage / JSON.parse exceptions thrown raw to callers CWE-209, CWE-755 V-222610, V-222656 SI-11, SA-15(5) II not-satisfied -> remediated
F-09 utils/textHighlight/TextHighlight.vue:24,63-65 Highlight term inserted into v-html markup unescaped and used as an unescaped regex CWE-79, CWE-1333 V-222602 SI-10 I not-satisfied -> remediated
F-10 13 innerHTML sites (listed in doc) Empty-string or static-template assignments, no user data CWE-79 V-222602 SI-10 I satisfied
F-11 formActions/CreateAction.js:94 Error saving objects: ${err} shown to operator CWE-209 V-222610 SI-11 II not-satisfied -> remediated
F-12 ui/layout/AboutDialog.vue:30 v-html of deployer Branding.aboutHtml (configuration, not user data) CWE-79 V-222602 SI-10 I satisfied
F-13 UserAPI.js:135, FaultManagementAPI.js:122-171, import/export actions, notebook-entries.js:242-356, NotebookComponent.vue:586 No audit records for import, export, role change, notebook entry create/delete, fault ack/shelve CWE-778 V-222471, V-222472, V-222476, V-222477 AU-2, AU-3, AU-12 II not-satisfied -> remediated
F-14 URLIndicatorPlugin/URLIndicator.js:78,100 Polls deployer-configured URL; scheme is configuration CWE-319 V-222596 SC-8 I needs-input
F-15 persistence/couch/plugin.js:36-68, CouchObjectProvider.js:218 CouchDB TLS termination/version not visible in source CWE-319 V-222596, V-222597 SC-8, SC-13 I needs-input
F-16 CouchObjectProvider.js:218 No embedded credentials (satisfied, V-222642); CouchDB authn/authz is deployment CWE-306 V-222522, V-222642 IA-2, IA-5(7), AC-3 I needs-input
F-17 example/exampleUser/ExampleUserProvider.js:96-240, userIndicator, operatorStatus, api/user Example provider is in-memory with auto-login; not production authentication. Status/poll inputs use text interpolation (satisfied for XSS) CWE-287, CWE-285 V-222522, V-222425 IA-2, AC-3 I needs-input; example provider not-applicable
F-18 LocalStorageObjectProvider.js, api/user/StoragePersistence.js:27-33 Objects and active role stored unencrypted in localStorage; CouchDB at-rest is server-side CWE-312 V-222587, V-222588 SC-28 II/I needs-input
F-19 deployment (proxy / IdP) Session IDs, idle timeout, logoff, cookie flags not in this client CWE-613 V-222577, V-222389, V-222391, V-222388 AC-12, SC-23 I/II needs-input
F-20 api/audit/AuditLogger.js Audit records in-process only; centralized forwarding is deployment CWE-778 V-222475 AU-4, AU-6, AU-9 II needs-input
F-21 condition/utils/operations.js:45, summaryWidget/src/ConditionEvaluator.js:344-413 User-authored criteria evaluated via fixed operation table; no eval/new Function CWE-94 V-222609, V-222604 SI-10, CM-7 I satisfied
F-22 hyperlink/HyperlinkLayout.vue:50 href passed through @braintree/sanitize-url CWE-79 V-222602 SI-10 I satisfied
F-23 notebook/components/NotebookEmbed.vue:31, notebook-image.js Snapshot images bound via :src from app-generated captures CWE-79 V-222602 SI-10 I satisfied
F-24 NotebookEntry.vue:684 Entry text stripped to plain text before storage CWE-79 V-222606 SI-10 II satisfied
F-25 api/objects/ObjectAPI.js:441 Conflict message includes only object key-string CWE-209 V-222600 SI-11 II satisfied

Counts: 11 not-satisfied -> remediated; 7 satisfied; 7 needs-input; 1 not-applicable; 0 not-satisfied left open.

Remediated in this PR

Change Specs
R1 importValidation.js (new): rejects __proto__/constructor/prototype at any depth, validates root, identifier/key-string consistency, type/name/location, composition and condition-set references; ImportFromJSONAction validates the whole tree before the first save(), shows fixed messages, logs diagnostics, and emits import audit records importValidationSpec.js, ImportFromJSONActionSpec.js
R2 NotebookEntry.vue: sanitize-html schema restricted to http/https/mailto, no protocol-relative URLs, enumerated attributes; link renderer requires http(s):, exact host or dot-delimited subdomain, escapes text and href, adds rel="noopener noreferrer". TextHighlight.vue: HTML- and regex-escaped term, escaped class, $1 reinsertion NotebookEntrySpec.js (new), TextHighlightSpec.js (new)
R3 PersistenceError (new, exposed as openmct.objects.errors.Persistence); Couch and LocalStorage providers normalize failures to it with raw details in console.error; Conflict preserved; generic messages in export, notebook image and CreateAction paths couch/pluginSpec.js, localStorage/pluginSpec.js, ExportAsJSONActionSpec.js, CreateActionSpec.js
R4 AuditLogger (new) registered as openmct.audit; records {id, source, timestamp, action, outcome, actor{id,username,role}, target, details} to subscribers via addProvider({ record }); hooks in UserAPI.setActiveRole, FaultManagementAPI.acknowledgeFault/shelveFault, import/export actions, notebook entry create/delete. No external sink AuditLoggerSpec.js, UserAPISpec.js, FaultManagementAPISpec.js, notebook-entriesSpec.js

Root cause for the error-message findings (F-02/04/06/07/08/11): the raw-error-to-operator pattern was repeated at six sites with no central rule. R3 centralizes provider failures in PersistenceError so any caller displaying error.message from the object API now shows generic text; the UI-level sites were fixed individually. Audit hooks were placed at the API layer where one exists (UserAPI, FaultManagementAPI) so every caller is covered.

Documented / needs-input

Evidence an ISSM/assessor must supply (details in the doc): CouchDB TLS configuration and version policy (F-15); URL indicator target scheme (F-14); CouchDB authentication and _security role model plus the production UserProvider/IdP/MFA (F-16, F-17); proxy/IdP session timeout, logoff and cookie flags (F-19); data-sensitivity determination for browser localStorage and CouchDB host encryption (F-18); reverse-proxy security headers (CSP, HSTS, X-Content-Type-Options, frame-ancestors); an openmct.audit provider forwarding to the site log store (F-20).

Test baseline comparison

Run npm run lint npm test Failing specs
master (a4aae41af) pass 6 FAILED, 969 SUCCESS Object API Search x4, Image Exporter x1, URLIndicator default icon class x1
this branch (7c8deeb07) pass (eslint js+vue, cspell) 6 FAILED, 1048 SUCCESS same 6
this branch (final) pass (eslint js+vue, cspell) 0 FAILED, 1061 SUCCESS (67 skipped) none

The six baseline specs are environment-sensitive, are not touched by this PR, and did not reproduce on the final run (CI unit-test also passes); +92 specs, no regressions. Notebook e2e link tests run locally via npm run test:e2e:ci -- tests/functional/plugins/notebook/notebook.e2e.spec.js: 17 passed / 22 skipped (upstream fixme), no e2e test edited. cspell entries added: STIG, DISA, ISSM, remediations, rethrew, notexample, Deployers, stig.

Original prompt

Do an Application Security & Development STIG and NIST SP 800-53 Rev. 5 review of Open MCT's untrusted-input boundaries (import/export JSON, notebook entries, URL plugins, CouchDB/localStorage persistence, user/role plugins, expression evaluation). Produce a findings table with file:line, CWE, ASD STIG rule ID, NIST control, severity and an explicit outcome for every boundary (satisfied / not-satisfied / needs-input / not-applicable). Then remediate the top findings in one PR with unit tests: schema validation and prototype-pollution protection on import, XSS sanitization on notebook sinks, generic error handling on persistence failures, and a structured audit-event hook for operator actions. Keep lint and the test suite green and show a baseline comparison.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this a notable change that will require a special callout in the release notes? For example, will this break compatibility with existing APIs or projects that consume these plugins? Yes: openmct.audit is a new API; persistence providers now reject with openmct.objects.errors.Persistence instead of raw fetch/browser errors; Import-from-JSON now rejects trees that previously imported partially.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Has this been smoke tested? (Karma unit specs only; no browser smoke test performed)
  • Have you associated this PR with a type: label? Note: this is not necessarily the same as the original issue.
  • Have you associated a milestone with this PR? Note: leave blank if unsure.
  • Testing instructions included in associated issue OR is this a dependency/testcase change? (npm run lint && npm test)

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?

Link to Devin session: https://app.devin.ai/sessions/9a388bf2196f49ee9af1542b613e2510
Open in Devin Desktop: https://app.devin.ai/desktop/session/9a388bf2196f49ee9af1542b613e2510?variant=devin
Requested by: @erikrozi


Devin Review

- Validate Import-from-JSON trees (reserved keys, identifiers, types,
  composition and condition-set references) before persisting; generic
  operator-facing error messages
- Sanitize Notebook Markdown links and TextHighlight output against XSS
- Normalize CouchDB / LocalStorage failures to a generic PersistenceError
  with raw details kept in console logging
- Add an in-process AuditLogger service (openmct.audit) with hooks for
  import, export, role change, notebook entry create/delete, and fault
  acknowledge/shelve
- Add docs/security/asd-stig-nist-800-53-review.md with findings and
  explicit per-boundary outcomes

Co-Authored-By: Erik Rozi <erik.rozi@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

Original prompt from Devin Bot

Repository: COG-GTM/openmct (fork of NASA Open MCT, branch master). Clone it, read README.md, CONTRIBUTING.md, TESTING.md, SECURITY.md, and AGENTS.md if present. Read the already-open security PRs on this fork so you do not duplicate them: #10 (Comps SharedWorker mathjs hardening), #11 (Web Page iframe sandbox / URL allowlist), #12 (npm registry signatures), #13 (CSV formula injection). Your work must cover DIFFERENT findings.

You have organization knowledge titled "Federal Security Compliance Knowledge" (STIG V-2206xx ↔ NIST 800-53 mappings, secure coding requirements). Use it as your control vocabulary. The repo COG-GTM/federal_security_compliance also contains devin/knowledge.md, devin/playbook.md and a NIST 800-53 scan playbook — read them before you start.

#``# Task

Perform an Application Security and Development (ASD) STIG + NIST SP 800-53 Rev. 5 code review of Open MCT's untrusted-input and data-boundary surfaces, then remediate the highest-value findings in one PR with tests.

Part A — Assessment (write it as docs/security/asd-stig-nist-800-53-review.md in the repo, and also summarize in the PR description):

  1. Enumerate the trust boundaries: Import-from-JSON action (src/plugins/importFromJSONAction), Export-as-JSON, Notebook entries and embeds (src/plugins/notebook), Hyperlink / URL indicator plugins, CouchDB persistence provider (src/plugins/persistence/couch), LocalStorage persistence, the Example User / operator-status / role model (example/exampleUser, src/plugins/userIndicator, src/plugins/operatorStatus), Condition Sets / Summary Widgets that evaluate user-authored expressions, and any innerHTML/v-html usage.
  2. For each boundary, record findings in a table: ID, file:line, description, CWE, ASD STIG rule (use real STIG rule IDs such as V-222602 input validation, V-222609 XSS, V-222612 / V-222601 error handling, V-222604 audit records, V-222577 authenticated sessions, V-222596 encrypted transport, etc. — look up the exact cur... (3958 chars truncated...)

@devin-ai-integration devin-ai-integration Bot added type:maintenance Maintenance/CI change no milestone PR intentionally has no milestone labels Sep 7, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 5 potential issues.

Devin Review

Comment thread src/plugins/importFromJSONAction/ImportFromJSONAction.js Outdated
Comment thread src/plugins/notebook/utils/notebook-entries.js
Comment thread src/api/audit/AuditLogger.js
Comment thread src/MCT.js
Comment on lines +199 to +205
/**
* Structured audit records (who / what / when / outcome) for operator
* actions. Providers subscribe to receive records; nothing is persisted
* or transmitted by default.
* @type {AuditLogger}
*/
this.audit = new AuditLogger(this);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Senior API approval required

The new public openmct.audit API requires senior-developer approval under CONTRIBUTING.md. Confirm that approval before merge.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. openmct.audit is a new public API and per CONTRIBUTING.md needs senior-developer approval before merge; this PR is intentionally left unmerged for that review. The API surface is small (record, addProvider, removeProvider, hasProviders, EventEmitter on/off/once) and documented in API.md.

Comment thread src/plugins/importFromJSONAction/importValidation.js
…tions; bound import traversal; isolate audit provider failures

Co-Authored-By: Erik Rozi <erik.rozi@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown

Browser evidence: import validation, Notebook XSS sanitization, audit records

Driven through the real UI at http://localhost:8080 (LocalStorage persistence, npm start) on this branch, with ExampleUser (operator-one, roles flight/driver) and ExampleFaultSource enabled temporarily for the role-change and fault scenarios (reverted afterwards; no index.html diff).

Annotated recording (2 min): security-browser-tests-edited.mp4 · full write-up: testing-report.md

Recording preview

Scenarios

# Scenario Result
1 Import __proto__ payload Generic "import rejected" notification; reasons only in console; no object created, no LocalStorage change, ({}).polluted === undefined
2 Import constructor.prototype payload Same as 1
3 Import identifier / key-string mismatch Rejected generically; no persistence
4 Import referencing a composition child absent from the payload Accepted (see note below)
5 Export folder + child, re-import into another folder Roundtrip succeeds; both objects present
6 Notebook entry: <img src=x onerror=alert(1)> <script>…</script> [click](javascript:…) **bold** No alert; rendered as <p> click ok bad <strong>bold</strong></p>; no img/script/a elements; https:, javascript: and data: links become plain text when no allowlist is configured
7 Search highlighting with <b>x and ( queries Literal text, no markup injection, no regex error
8 Audit records for export, import, notebook entry create/delete, role change (flight → driver), fault acknowledge, fault shelve One record each with id, source, timestamp (ISO UTC), action, outcome, actor{id,username,role}, details
9 Storage.setItem forced to throw QuotaExceededError internal-path-secret Operator sees only "Error saving objects"; the secret string appears in console diagnostics only
Rejected imports (generic message, clean prototype) Notebook payload rendered inert
Rejected imports Notebook safe rendering
Audit records + required-field checks Generic save error shown to operator
Audit records Generic save error
Detailed error confined to console Export/import roundtrip
Console diagnostics Roundtrip

Findings from the browser run and their disposition

  1. Rejected imports emitted no import/failure audit record when the file was rejected by the form validator (before OK is enabled). Fixed in 887b66c: the form validator now routes through _rejectImport(), so the generic notification and the failure audit record are emitted on the same path as save-time rejections. Covered by a new Karma spec (ImportFromJSONActionSpec.js, "rejects invalid files in the form validator…").
  2. Composition reference to an object absent from the payload was accepted (rendered as Missing: absent / Unknown Type). Kept as-is on purpose: ExportAsJSONAction#exportObject deliberately omits non-creatable children (e.g. read-only telemetry) while leaving their identifiers in the parent's composition, so a valid export can legitimately contain such references. Rejecting them would break real roundtrips; the reference is validated for shape and is never dereferenced as code. Documented in docs/security/asd-stig-nist-800-53-review.md (F-01).

CI follow-ups in this push (887b66c)

  • e2e-ci shard 2/4 (Notebook link tests, regression from R2): root cause was rendering url.href (canonicalised, e.g. trailing /, empty-query normalisation) instead of the original href in the generated anchor, so the upstream selectors that match on the literal text no longer found the anchor. The allowlist rule (exact host or dot-delimited subdomain) was not the cause — google.com correctly admits bing.google.com and www.google.com. Fix: validate via new URL(href) but render the escaped original string. Local run of npm run test:e2e:ci -- tests/functional/plugins/notebook/notebook.e2e.spec.js: 17 passed / 22 skipped (skipped = upstream test.fixme), including lines 429, 480, 494 and 517. No e2e test was edited.
  • visual-a11y-ci (1) "main view" colour-contrast (#050505 on #000000): not caused by this branch. Evidence: the same head commit 7c8deeb ran the job four times (runs 34164649617, 34164721574, 34164927696 → pass; 34164955621 → fail), and the identical violation appeared on an unrelated branch on the same base (run 34163719018, devin/1788814796-flight-test-telemetry-plugin, job 101871007996). The flagged elements are the Inspector pane labels while the pane is collapsing, i.e. an animation-timing flake in the upstream test.
  • e2e-couchdb: pre-existing displayLayout @couchdb failure, identical on Add html and text-summary coverage reporters to karma config #15/Add Flight Test Telemetry example plugin (PCM, MIL-STD-1553, TSPI, faults, IRIG 106 Ch10 adapter) #16; not addressed.
  • Also in this push (from the automated review): bounded import traversal (64 nesting levels / 20 collected errors) and isolation of throwing or rejecting audit listeners/providers. Lint clean; npm test → 1061 passed / 0 failed / 67 skipped (base: 969 passed).

Written by Devin

Co-Authored-By: Erik Rozi <erik.rozi@cognition.ai>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 9 new potential issues.

Devin Review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Required issue link is missing

The contribution rules require every pull request to link an issue. The description uses “Closes N/A,” leaving no required traceability record.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no tracking issue on this fork for this review; the work was requested directly, so "Closes N/A" is deliberate. If a tracking issue is opened I will link it in the description.

Comment thread src/plugins/exportAsJSONAction/ExportAsJSONAction.js
Comment thread src/plugins/notebook/utils/notebook-entries.js
Comment thread src/api/user/UserAPI.js Outdated
Comment thread src/plugins/localStorage/LocalStorageObjectProvider.js
Comment thread src/api/audit/AuditLogger.js Outdated
Comment thread src/plugins/importFromJSONAction/ImportFromJSONAction.js
Comment thread src/MCT.js
Comment thread src/plugins/importFromJSONAction/importValidation.js
…guard localStorage getter; emit audit events via EventEmitter; document Audit API

Co-Authored-By: Erik Rozi <erik.rozi@cognition.ai>
devin-ai-integration[bot]

This comment was marked as resolved.

- ExportAsJSONAction: nested #exportObject writes are returned so a
  descendant load failure reaches the generic handler (dialog closes,
  generic notification, failure audit record)
- Notebook createNewImageEmbed resolves undefined and notifies the
  operator on FileReader, thumbnail or persistence failure instead of
  staying pending; consumers skip undefined embeds
- ImportFromJSONAction waits for every save to settle before reporting
  a failure and never links the root on partial failure
- AuditLogger.record() resolves after asynchronous providers settle;
  provider rejections stay isolated; API.md documents the contract
- importValidation documents why type keys are allowlisted and adds a
  spec pinning plugin-style type keys, namespaced identifiers and deep
  plugin configuration as accepted

Co-Authored-By: Erik Rozi <erik.rozi@cognition.ai>
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration Bot and others added 3 commits September 7, 2026 22:57
… import objects

Co-Authored-By: Erik Rozi <erik.rozi@cognition.ai>
Co-Authored-By: Erik Rozi <erik.rozi@cognition.ai>
Co-Authored-By: Erik Rozi <erik.rozi@cognition.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no milestone PR intentionally has no milestone type:maintenance Maintenance/CI change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant